Add fixed-hash and uint crates to parity-common #12

dvdplm · 2018-07-13T08:53:57Z

Both susbtrate and ethereum-types use fixed-hash to build their hash types so it's getting moved here. uint is also moved as it is used in a few places in substrate.

The new code here is from d2e4ec5 and fixes a compiler warning and adds a test.

One thing I am not happy with is that uints tests have to be run with cargo test --features=std,impl_quickcheck_arbitrary. Advice on how to fix that so cargo test works is much appreciated.

* add rustc_version to build deps of ether types (needed by build script) * ethereum-types: add build script that sets cargo:rustc-cfg=asm_available on nightly * ethereum-types: #![cfg_attr(asm_available, feature(asm))] * tests: add the whole feature asm build script to make it pass on nightly CI * add #![cfg_attr(asm_available, feature(asm))] directly where needed so users don't have to add it * remove tests/build.rs since it should not be needed anymore * move inner cfg_attr attributes to the right allowed position * remove check for asm_available in contexts that already have check for that

* feature(asm) is only allowed in crate * add tests/build.rs again - haven't found a way around it

tomusdrw · 2018-07-13T09:07:44Z

I don't really get the reasoning for the move. The crate only exposes a macro to generate your own types, so it's only referenced by at most one crate.

paritytech/primitives already contains all the crates required to build uint/hashes, and doesn't depend on other git repos. Now ethereum_types for instance have to pull in parity-common repo?

None of the crates in this repo will ever dependend on fixed-hash, so I don't get what do we gain by having it here instead of in primitives.

IMHO we etiher keep parity-common and primitives separate or we dissolve primitives and move ALL crates here. Moving only fixed-hash doesn't make much sense to me.

git-subtree-dir: uint git-subtree-mainline: d2e4ec5 git-subtree-split: 0435699

Fix compiler warning Bump version

git-subtree-dir: uint/tests git-subtree-mainline: 9144702 git-subtree-split: 716691c

Fix wonky example causing long compile times

dvdplm · 2018-07-13T16:50:57Z

fixed-hash/src/lib.rs

+
+#[cfg(feature="std")]
+#[doc(hidden)]
+pub extern crate core;


Why are all these crates pub?

dvdplm · 2018-07-13T16:51:22Z

uint/README.md

+implementations for even more speed, hidden behind the `x64_arithmetic`
+feature flag.
+
+Run tests with `cargo test --features=std,impl_quickcheck_arbitrary`.


How can I improve this? :/

Does https://doc.rust-lang.org/cargo/reference/manifest.html#the-required-features-field-optional help?

I tried that and no, it's not really doing what I'd like. With:

[[test]] name = "uint_tests" required-features = ["std,impl_quickcheck_arbitrary"]

…I can run cargo test and all tests are skipped which is better than spewing errors, but now cargo test --features=std,impl_quickcheck_arbitrary also skips all tests.

What I'm looking for is a way to tell cargo "when running cargo test use this set of features: … … ".

UPDATE: seems to be a known limitation

twittner · 2018-07-16T08:31:42Z

Like @tomusdrw I am somewhat puzzled by this. Would the motivation behind parity-common not make it logical to put all primitive crates in here? But what prevents parity-common from becoming a black hole of crates? What about other repositories, e.g. paritytech/exit-future, paritytech/bigint, etc.? I guess the question I have is: When should a crate be a member of parity-common instead of being in a separate repository?

dvdplm · 2018-07-30T10:01:12Z

Would the motivation behind parity-common not make it logical to put all primitive crates in here?

Some of the code in primitives is ethereum-specific and should thus not be in parity-common.

what prevents parity-common from becoming a black hole of crates?

This is a great question and a concern I have too. The answer is, I guess, "we're disciplined about what we move". And "one could argue that parity-ethereum is already a black hole of crates and at least this new black hole is built with the strict requirement of code being used in more than one project".

The background conversation here is the tension between two valid but conflicting ergonomical concerns:

splitting out code into own repos is great for consumers (easy to version, publish, and pull into other projects)
maintaining and updating a crate is much more comfortable when changes can be made in a single repo and with a single PR

The parity-common effort tries to find a middle ground between the two. And as mentioned, the criterium used is pretty pragmatic: "crates used in more than one Parity project that are not not already split out into their own repos".
As mentioned above I am also worried that we'll end up with big pile of unrelated crates and be back at square one, but I am also convicned that leaving things as they are is not a good option: we really really don't want to maintain forks and/or risk loosing track of what code is used where.

ordian · 2018-07-30T13:25:39Z

uint/tests/uint_tests.rs

 fn uint256_mul32_old() {
+	assert_eq!(U256::from(0u64).mul_u32(2), U256::from(0u64));


Accidentally duplicated line

dvdplm · 2018-08-01T14:12:04Z

@tomusdrw @twittner @debris do you still have concerns about this?

debris and others added 30 commits October 24, 2017 16:09

ethereum-types refactor

c110f3e

ethereum-types refactor

c4e00f8

ethereum-types refactor

c3b2b5c

ethereum-types refactor

9dec8ff

ethereum-types refactor

845d305

unify hash macro by renaming it to construct_hash

91b87cd

unify hash macro by renaming it to construct_hash

2791a94

feature(asm) is only allowed in crate (#8)

45f721c

* feature(asm) is only allowed in crate * add tests/build.rs again - haven't found a way around it

feature(asm) is only allowed in crate (#8)

d0f9df0

* feature(asm) is only allowed in crate * add tests/build.rs again - haven't found a way around it

updated fixed-hash manifest

71b84a2

updated uint manifest

9e23e8f

fixed-hash implements heap-size, version 0.1.1

7bae799

ethbloom crate

f1e6fbe

ethbloom crate

2ea80de

fixed-hash 0.1.2

d741c2f

add default serde serialization to ethereum-types

0aab3c4

add default serde serialization to ethereum-types

4c709de

Checked ops.

ea29612

Checked ops.

61cbcd7

LowerHex and small cleanup.

6daac62

LowerHex and small cleanup.

be5ce47

LowerHex and small cleanup.

8e5fd03

Fix indentation.

447984a

Fix indentation.

d0e82be

Fix indentation.

978bd14

Remove 0x to be aligned with std LowerHex.

74a227f

Remove 0x to be aligned with std LowerHex.

68d0fab

use ASM only with feature

ceb5f74

use ASM only with feature

3059ca7

dvdplm added A0-pleasereview F5-task M5-dependencies labels Jul 13, 2018

dvdplm requested review from twittner and gavofyork July 13, 2018 08:56

dvdplm mentioned this pull request Jul 13, 2018

Move fixed-hash and uint to parity-common; remove tests crate paritytech/primitives#47

Merged

dvdplm added the A1-onice label Jul 13, 2018

dvdplm added 7 commits July 13, 2018 12:03

Add 'uint/' from commit '043569906ba7a817abde33877fe1f71cdbc87749'

9c065b7

git-subtree-dir: uint git-subtree-mainline: d2e4ec5 git-subtree-split: 0435699

Add uint to workspace

bd8ea85

Fix compiler warning Bump version

Remove uint (moved to parity-common)

716691c

Move formatting test from primitives

9144702

Add 'uint/tests/' from commit '716691c42b794ad004d95df5ecdc77fe7d3b9d3b'

646fe8c

git-subtree-dir: uint/tests git-subtree-mainline: 9144702 git-subtree-split: 716691c

Move tests from primitives/tests to uint

f8ee2e6

Fix wonky example causing long compile times

cleanup

70a778a

dvdplm changed the title ~~Add fixed-hash to parity-common~~ Add fixed-hash and uint crates to parity-common Jul 13, 2018

dvdplm commented Jul 13, 2018

View reviewed changes

fixed-hash/src/lib.rs

#[cfg(feature="std")]

#[doc(hidden)]

pub extern crate core;

Copy link

Contributor Author

dvdplm Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are all these crates pub?

dvdplm commented Jul 13, 2018

View reviewed changes

Fix travis

4e4347f

dvdplm removed the A1-onice label Jul 13, 2018

Silence warnings when running tests

dfc1c79

ordian reviewed Jul 30, 2018

View reviewed changes

Remove duplicated line

b0cf5d1

tomusdrw approved these changes Aug 2, 2018

View reviewed changes

debris approved these changes Aug 3, 2018

View reviewed changes

dvdplm merged commit ae80aff into master Aug 3, 2018

debris deleted the add-fixed-hash branch August 6, 2018 08:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fixed-hash and uint crates to parity-common #12

Add fixed-hash and uint crates to parity-common #12

dvdplm commented Jul 13, 2018 •

edited

Loading

tomusdrw commented Jul 13, 2018

dvdplm Jul 13, 2018

dvdplm Jul 13, 2018

twittner Jul 16, 2018

dvdplm Jul 30, 2018

dvdplm Jul 30, 2018

twittner commented Jul 16, 2018

dvdplm commented Jul 30, 2018

ordian Jul 30, 2018

dvdplm commented Aug 1, 2018

		fn uint256_mul32_old() {
		assert_eq!(U256::from(0u64).mul_u32(2), U256::from(0u64));

Add fixed-hash and uint crates to parity-common #12

Add fixed-hash and uint crates to parity-common #12

Conversation

dvdplm commented Jul 13, 2018 • edited Loading

tomusdrw commented Jul 13, 2018

dvdplm Jul 13, 2018

Choose a reason for hiding this comment

dvdplm Jul 13, 2018

Choose a reason for hiding this comment

twittner Jul 16, 2018

Choose a reason for hiding this comment

dvdplm Jul 30, 2018

Choose a reason for hiding this comment

dvdplm Jul 30, 2018

Choose a reason for hiding this comment

twittner commented Jul 16, 2018

dvdplm commented Jul 30, 2018

ordian Jul 30, 2018

Choose a reason for hiding this comment

dvdplm commented Aug 1, 2018

dvdplm commented Jul 13, 2018 •

edited

Loading